Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

event: minimize the size of structures by field order #10

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

algol83
Copy link

@algol83 algol83 commented Jun 7, 2024

V802. On 32-bit/64-bit platform, structure size can be reduced from N to K bytes by rearranging the fields according to their sizes in decreasing order https://pvs-studio.com/en/docs/warnings/v802/

@aikuchin
Copy link
Contributor

When PVS suggests that something can be done it doesn't necessarily mean that it should be done.
There are not many allocations of these structures and since the library heavily relies on multythreading false sharing can be much bigger problem than a few wasted bytes. And IMHO even readability and ease of understanding structure logic are more important in this case.

Is there a known problem with memory consumption by these structures?
Did you measure how much memory this patch saves and how it impacts performance?

V802. On 32-bit/64-bit platform, structure size can be reduced from N to K bytes by rearranging the fields according to their sizes in decreasing order
https://pvs-studio.com/en/docs/warnings/v802/

Signed-off-by: Aleksandr Golubev <[email protected]>
@d-tatianin
Copy link
Collaborator

When PVS suggests that something can be done it doesn't necessarily mean that it should be done. There are not many allocations of these structures and since the library heavily relies on multythreading false sharing can be much bigger problem than a few wasted bytes. And IMHO even readability and ease of understanding structure logic are more important in this case.

I generally agree with the false sharing statement, but both of these structures are less than a cache line in size, so false sharing doesn't really go away either way, and this way we win a few bytes for free. Unless you have specific readability concerns with this, I would merge it.

@aikuchin
Copy link
Contributor

aikuchin commented Jun 17, 2024

Unless you have specific readability concerns with this, I would merge it.

Yeah, sure. I don't see much profit from this, but I also don't expect any harm.

@d-tatianin d-tatianin merged commit 2af8c41 into yandex-cloud:master Jun 18, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants